-
-
Notifications
You must be signed in to change notification settings - Fork 574
Add HeyBoxChat to the list of supported providers #2374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Gehongyan <[email protected]>
Docs
Hints
Note Question 1: Of the 7 additional query parameters that need to be passed, "chat_version" may change as their product updates, while the other fields remain the same for requests coming from a Bot. Do we need to introduce a setting for "chat_version"? Useful Screenshots (Machine translated from Chinese) |
Thanks for your PR! ❤️
👍🏻
Hum, good question. If that "chat version" thing is like an API version, we usually don't expose that as a setting: instead, it's purely an internal thing we update when the schema changes and requires changing something in our implementation. FYI: I see you blurred the openiddict-core/src/OpenIddict.Client/OpenIddictClientConfiguration.cs Lines 309 to 341 in 4902658
|
Signed-off-by: Gehongyan <[email protected]>
I have received news from their development team that one day those query parameters will no longer be required, but it is still uncertain when this change will happen. When they update it, they will inform me, and I will also update it here. |
My pleasure! ❤️
👍🏻 Do you think we can ship this provider as-is (or maybe with the parameters added to the userinfo request, just in case) or do you prefer waiting until they change their implementation? 7.1.0 is around the corner, so if you prefer the first option, it may ship quickly 😃 |
Sounds great! I will make another pull request to update the code if they make any further changes. |
Signed-off-by: Gehongyan <[email protected]>
Now, only |
Should we also add these parameters to the userinfo request, tho' you said they are not currently required in practice? Just in case, you know 😄 |
Regarding the name, do we know what the official casing is? A quick search on Google doesn't return a lot of results (I guess the Chinese name is a lot more popular?), but "Heybox Chat" seems to be the most common result. If it's the correct casing, we should probably name the provider |
I confirmed with the official development team earlier today that the
Before I opened this pull request, I had asked them what English names they prefer. The answer was that they do not have an official English name, and
About the parameter necessities and official English names, I will communicate with the development and operations teams tomorrow for confirmation. If you need to release a new version as soon as possible, it is okay to postpone completing this pull request, since it is not urgent. |
They have confirmed that the userinfo API does not require those parameters. This API is located under the /api request path, and APIs under this path will not be reused with requests from the HeyBoxChat client and web with user login info. Therefore, there is no need to differentiate the source, and those parameters are not needed.
They have confirmed the name HeyBoxChat, rather than other spellings or variations in capitalization. Based on their reply, this should be regarded as a proper noun as a whole. Based on the above, it seems that there is currently no code that needs further modification. Of course, if you have any ideas or hold certain opinions, I am also happy to listen to and respect them. 😄 |
👍🏻
Perfect! Thanks for taking the time to deal with that! It's nice to see they positively replied to your demand 😄
Great, let's merge it then! ❤️ |
Merged, thanks a lot for this great contribution! 👏🏻 |
Hey @gehongyan, FYI, 7.1.0 just shipped with your new provider: https://github.com/openiddict/openiddict-core/releases/tag/7.1.0 🎉 |
This pull request would like to add HeyBoxChat to the list of supported providers.